Add threadsafe dynamic direct peer handling to GossipSub#673
Add threadsafe dynamic direct peer handling to GossipSub#673cortze wants to merge 7 commits intolibp2p:masterfrom
Conversation
remove race condition on direct peering test rm missing race-condition
cortze
force-pushed
the
allow-dynamic-direct-peers
branch
from
943b2e8 to
c24fe95
Compare
|
I pushed the latest commit @cortze, please take a look |
|
I just fixed the test and covered a possible nil pointer given to the This should be ready to merge, but let me know if I should address anything else ✌🏽 |
tag_tracer.go
Outdated
| t.idGen = gs.p.idGen | ||
| t.direct = gs.direct | ||
| if gs.direct != nil { | ||
| t.direct = gs.direct |
There was a problem hiding this comment.
Does this rely on having a stable pointer to t.direct?
There was a problem hiding this comment.
is it intentional that this aliases to the gossipsub map?
There was a problem hiding this comment.
I don't like that it aliases to the gossipsub map. It means that if gs does not initialize the map this will be a nil pointer that will be dereferenced later. I've changed it to use a function instead.
tag_tracer.go
Outdated
| func (t *tagTracer) addDirectPeer(p peer.ID) { | ||
| t.Lock() | ||
| defer t.Unlock() | ||
| t.direct[p] = struct{}{} |
There was a problem hiding this comment.
Do we need to do this if this is aliased to the gossipsub map?
tag_tracer.go
Outdated
| } | ||
|
|
||
| func (t *tagTracer) addDirectPeer(p peer.ID) { | ||
| t.Lock() |
There was a problem hiding this comment.
I don't think we need the lock here.
3 participants
Description
The current direct peer handling is a bit clunky, it only allows direct peers to be set at the start of the service. This makes the application layer unable to modify the direct peer list, or to create new services as we want to add/remove peers to that list.
The PR adds two new self-descriptive methods to the
GossipSubRouterstruct:AddDirectPeer()RemoveDirectPeer()Both handling the underlaying
PeerstoreandTagTracerchanges as well. Ensuring that we have all in place to ensure the connection protected with the remote peer.